Repository navigation
fix(core): a decoded buffer's handover leaves the routed element silent - #7309
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at f865165. Requesting changes. There are two blockers.
1. Scrub audition goes silent again in Safari after a handover (regresses #4517).
#4517 set a rule: a captured element is connected straight to the destination whenever no play is running, so Studio's scrub audition, which plays the element directly under a paused clock, is audible. stopAll() keeps that rule by calling dispose(source) with keepsSounding = true on media-element sources.
After this change, a handed-over element breaks the rule:
releaseRoutedremoves the element's media-element source from_activeSourcesand leaves its node disconnected.- On Pause or seek,
stopAll()only sees the buffer source.disposestops the buffer and puts backel.muted, but it never reconnects_mediaElementSources.get(el). - The element is now unmuted, captured and connected to nothing. The next scrub audition plays into silence, which is exactly the -180 dB case in #4517's table.
decodesFirst covers every non-looping WebKit <audio> element at rate 1 that fits the decode budget, which is the music-track case Studio auditions. So this hits Safari Studio users, not an edge case. The PR body's "rate change reschedules" path covers only the next play, not the paused state.
Suggested fix: keep the element disconnected while the buffer plays (which is the right fix for the double sound). When the buffer source that replaced it is disposed (stop or pause), reconnect the element's cached source node to the destination. One way is in dispose for a buffer source whose element is in _handedOver: connect its _mediaElementSources node to the destination when keepsSounding is true. Please add a test along the lines of: schedule → releaseRouted → stopAll() → expect the element's node to be connected to ctx.destination.
2. The Comments workflow fails. webAudioTransport.ts's comment share rises from 233 to 234 lines because the releaseRouted doc comment grew from one line to two. Get it back to one line (or fewer).
The rest checks out. The diagnosis of the double sound is sound, the fails without the fix: line names a real test that fails on main, and leaving the element disconnected during the buffer's play is the right change.
There was a problem hiding this comment.
Re-reviewed at 55badf2. Approving.
Blocker 1 (scrub audition after a handover) is resolved. stopAll() now reconnects every handed-over element's cached source node to the destination and clears the set. That puts back #4517's rule that, outside a play, an element sounds through the destination. It covers both a buffer that was still playing and one that had already ended, since the reconnect is driven by _handedOver, not by _activeSources.
- The new test
sounds a handed-over element through the output again once playback stopschecks exactly this. - On the next Play,
scheduleMediaElementPlaybackdisconnects the node before it builds the chain, and a second handover'sdispose(…, false)leaves it disconnected. So the echo fix holds and there's never a second path out.
Blocker 2 (comments) is resolved. The releaseRouted doc comment is back to one line, so the file's comment count matches its base. The Comments run on this head was still in progress when I reviewed.
Not blocking: _handedOver changed from a WeakSet to a Set so it can be iterated, which means it now holds strong references until the next stopAll(). An element swapped out with its scene therefore stays referenced until that stop, and is then connected to the destination. It's already paused by releaseDetachedMedia, so nothing is heard. Consider having releaseDetached() also delete detached elements from _handedOver, to match its "without sending it to the destination" contract.
CI, Safari audio and the other workflows were still running when I reviewed. This approval doesn't stand in for them.
Edit accuracy: accurate 267 (base branch 267), smooth 125 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (2)
|
…view to heygen-com#7250 Adopts heygen-com#7310 (MPEG-4 Part 2 / MJPEG get browser-playable copies), heygen-com#7316 (check_report counts the copies a check waited for) and heygen-com#7317 (render reads a composition src like the preview and refuses paths outside the project). Defers heygen-com#7309 (needs heygen-com#6912); heygen-com#7314 does not apply. PR axis advances to heygen-com#7250 (heygen-com#7243 closed unmerged, heygen-com#7249 open and pinned as monitored; monitored 96->97). Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
When a decoded buffer takes over a clip's sound in WebKit, the clip's media element is released without being left sounding:
releaseRoutednow disposes the routed source without reconnecting it to the audio destination.Why
On a long source in Safari, the first seconds of some takes sounded twice: one copy on time, a second 200 to 320 ms later. The element's own late restart (WebKit starts a media element's audio 150 to 400 ms late) kept sounding through the destination after the buffer had taken over, because releasing a routed element reconnected it there ("the element then sounds natively") and WebKit does not silence a captured element for
muted.Related work
Follows the buffer handover that moved the start of the sound onto the context clock. Refs the Safari audio and picture offset reports.
How
releaseRoutedcallsdispose(routed, false): while the buffer plays, the captured element has no output, which is silent, and the buffer carries the sound.stopAll(), which every pause and seek goes through, then reconnects each handed-over element's cached source node to the destination, so Studio's scrub audition (the element played directly under a paused clock) is audible, as the rule that a captured element is connected whenever no play runs requires. That holds whether the buffer was still playing or had already ended on its own. A later rate change reschedules the element throughscheduleMediaElementPlayback, which reuses the one source node per element and context.Test plan
webAudioTransport.test.ts:keeps a routed element silent once a decoded buffer plays in its placeandsounds a handed-over element through the output again once playback stops. fails without the fix: on main the first fails atexpect(node.connect).not.toHaveBeenCalled()(one call, to the destination); on the previous head of this PR the second fails with the node never connected to the destination. With both changeswebAudioTransport,webAudioTransportIdle,initandmediatests pass, 605 tests.Size
About 19 changed lines, one fix and its test: a lone fix that no open PR can carry (the only related PR is merged).